Skip to content

Conversation

@JoshuaKGoldberg
Copy link
Contributor

Prerequisites checklist

What is the purpose of this pull request?

Adds Knip to check for unused exports, unused dependencies, and missing dependencies

What changes did you make? (Give an overview)

  1. Adds Knip, similar to how it was done in the eslint/eslint repo in chore: Introduce Knip eslint#18005.
  2. Adds in devDependencies that it noted are relied upon but don't exist
  3. Adds -y to the jsr launch to explicitly always run it without prompting

Related Issues

Related to #462, which found types exported by the package but not used anywhere.

Is there anything you'd like reviewers to focus on?

@snitin315
Copy link
Contributor

Why is CI green when we have unused exports in the main branch, as per #462?

@lumirlumir
Copy link
Member

lumirlumir commented Jul 16, 2025

Hi @snitin315,

I'm not Joshua, but I found this comment in #462 😅

Unfortunately it can't detect this specific issue because types.ts is an entry point per package.json > "exports". From Knip's perspective, anything that is exported from an entry point is "used". Sent: #466.

Currently, since the types.d.ts file is exported via the exports field, I think Knip can't detect it as unused—it considers anything exported from an entry point as "used."

markdown/package.json

Lines 13 to 21 in 094a59d

"exports": {
".": {
"types": "./dist/esm/index.d.ts",
"default": "./dist/esm/index.js"
},
"./types": {
"types": "./dist/esm/types.d.ts"
}
},

Copy link
Member

@lumirlumir lumirlumir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today I learned what Knip does 😄

I'm in favor of this change 👍, but I'll move it to the Feedback Needed section for now to gather more input.

I've also left a minor comment.

@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Jul 16, 2025
@lumirlumir lumirlumir moved this from Needs Triage to Feedback Needed in Triage Jul 16, 2025
@lumirlumir
Copy link
Member

I’ll check back in once a decision has been made 😄

@nzakas
Copy link
Member

nzakas commented Jul 22, 2025

Generally I'm on the side of more tooling, I'd just like a better explanation of what prompted this PR and why the check is green if there are unused exports.

@github-actions
Copy link
Contributor

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Aug 13, 2025
@JoshuaKGoldberg
Copy link
Contributor Author

what prompted this PR

It's the two reasons noted in the OP:

  • chore: Introduce Knip eslint#18005: Knip is a generally useful tool that finds unused code. I figured I'd add it in now while the repo is relative younger and doesn't have as much cruft built up.
  • fix: remove unused types from types.ts #462: even though that specific classification of unused code can't be caught by Knip, the fact that there are things building up IMO is a signal that something like Knip would be useful

why the check is green if there are unused exports

It's #466 (comment) <- #462 (comment). The specific kind of issue in #462 -a type being exported out of the package but not used by any external consumers- is out of scope for Knip.

@github-actions github-actions bot removed the Stale label Oct 27, 2025
@lumirlumir
Copy link
Member

It seems that adding knip to the repositories is the approach the team wants to take.

Based on the references below, I'm marking this PR as accepted.


Hi, @JoshuaKGoldberg

When you have a moment, could you take a look at the merge conflicts and comments?

@lumirlumir lumirlumir moved this from Feedback Needed to Implementing in Triage Oct 30, 2025
@JoshuaKGoldberg JoshuaKGoldberg requested a review from a team as a code owner October 31, 2025 17:59
Co-authored-by: 루밀LuMir <[email protected]>
Copy link
Member

@lumirlumir lumirlumir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Since release-please.yml file has been changed, would like @eslint/eslint-tsc and others to verify before merging.

@lumirlumir lumirlumir moved this from Implementing to Second Review Needed in Triage Nov 2, 2025
@lumirlumir lumirlumir requested a review from a team November 2, 2025 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Second Review Needed

Development

Successfully merging this pull request may close these issues.

4 participants